Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for T420 and T430, enable FBWhiptail for X220. #539

Closed
wants to merge 24 commits into from

Conversation

snmcmillan
Copy link
Contributor

The T420 support is currently untested, but currently builds and should work.

Blob extraction scripts now autodetect coreboot's ifdtool and me_cleaner.

@snmcmillan
Copy link
Contributor Author

Alright, so I am going to do another build and test if T420 will work. Will update with results.

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Mar 21, 2019

Alright, T420 builds and boots. Over the weekend, I will do usability tests and see if there are any major issues with the build and try to iron them out.

Edit: So I've done a bit of testing and outside of the seemingly platform independent issue I referenced above, everything looks good.

@snmcmillan
Copy link
Contributor Author

One thing that's an issue currently is that ifdtool and me_cleaner are not present on a clean tree. You need to run a make job to get the coreboot repo cloned and then patch said repo.

@snmcmillan
Copy link
Contributor Author

Currently working on issue #541 as well as updating the X220 and T420 builds to FBWhiptail.

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Mar 27, 2019

FBWhiptail is now working on X220 and T420. I left X230 IOMMU kernel command line options enabled and I do not have working graphics in the current OS (Xubuntu 18.10). I'll commit changes when I get home today (in about 9 hours).

Edit: OK, so I managed to temporarily disable those config options (why do we copy /etc/config to /tmp/config? Seems silly since its already temporary). I've recovered from that roadblock and am back on the main system.

…estarting, or during S3 resume. If they actually bring functionality to builds, replace the patch with something that is tested.
@snmcmillan snmcmillan changed the title Add Support for T420 and fix blob extraction scripts. Add support for T420, make X220 and T420 use FBWhiptail, fix sandybridge 50 second delay. Mar 27, 2019
@snmcmillan
Copy link
Contributor Author

PR now solves #541.

Pull request is ready to be merged.

Copy link
Collaborator

@flammit flammit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping these sandybridge patches removes a core bit of functionality that makes Heads work (i.e. measuring the first few levels of coreboot segments). This will impact not just the x220, but also the x230.

NAK until we can figure out a performant replacement that works or doesn't impact other platforms.

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Mar 27, 2019

I'll continue taking a look at this, as the delay is a pretty big issue for other platforms.

Edit: This issue only affects non-X230 platforms. X230 has no delay, but literally any other sandy/ivy-based device has this delay.

@snmcmillan
Copy link
Contributor Author

@flammit I've found that the second tpm measurement in the romstage, precisely these lines are causing the issue:

extern char _romstage, _eromstage;
tlcl_measure(1, &_romstage, &_eromstage - &_romstage);

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Mar 27, 2019

This should keep the TPM measurements of the romstage as well as fix the delay, ready for review.

Edit: Before review, let me make some final changes.

@flammit
Copy link
Collaborator

flammit commented Mar 28, 2019

@SebastianMcMillan nice catch btw - try this patch out: https://gist.github.com/flammit/247ca7877f5f885b5d9bb5d9c56196ce. If it works for you, would you mind opening another PR for that so we can merge it separately from the x220 support?

@snmcmillan
Copy link
Contributor Author

I can do that, just recovered from accidentally enabling NO_GFX_INIT by blindly remoting into my main system and rebuilding. I can test right now...

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Mar 28, 2019

@flammit the patch you sent doesn't fix the problem (it even gives the same coreboot.rom checksum as the previous patch). I'll revert the sandy patch commits so T420 support and updating X220 to FBWhiptail changes can be merged.

Update: Ready for merge

…ng up, restarting, or during S3 resume. If they actually bring functionality to builds, replace the patch with something that is tested."

This reverts commit 41e5301.
@flammit
Copy link
Collaborator

flammit commented Mar 28, 2019

@SebastianMcMillan I see a change to the payload and hash when switch the patches. Remember that the Heads module patches are only applied when the module is first pulled, so you have to remove build/coreboot-4.8.1 and make again for the new patch to apply.

@tlaurion tlaurion self-assigned this May 22, 2019
@merge
Copy link
Contributor

merge commented May 22, 2019

IMO this should be tested by at least one of us who didn't write the changes. Also, I think it would make sense to stash the commits together nicely and improve the commit messages. Why do they mention "merging"?

@techge
Copy link
Contributor

techge commented May 22, 2019

I might be able to help. I just do not understand if this still is a fix for x220 too or if there is still problem with timeout or whatever. But I can test it for x220 if you are interested (actually I am waiting for being able to use it on my device).

Would I just checkout this pr and build it? Or is there anything else I need to do before flashing?

@tlaurion
Copy link
Collaborator

tlaurion commented May 22, 2019

@techge

I might be able to help. I just do not understand if this still is a fix for x220 too or if there is still problem with timeout or whatever. But I can test it for x220 if you are interested (actually I am waiting for being able to use it on my device).

The 50 second delay is still there and not well understood yet. From cbmem timestamp log the problem seems to be linked to Intel ME or TPM measurements, but neither with BBB Screwdriver USB debugging gadget or by inceasing cbmem logs were enough to get the culprit trace. USB dongle arrives too late, while cbmem maximum log length is not big enough to show early messages and get troncated.

I do not have a X220 in hands anymore to continue testing, but the same bug impacts all Ivy/Sandy bridge but x230. So anyone having those models and knowledge are more then welcome to jump in and troubleshoot the issue. Merging this makes those board supported, with 50 second delay at bootup and resume path (which makes me believe it's not a TPM issue, but Intel ME... While a pure coreboot build/removing TPM measurements patch removes the delay so it needs to be TPM measurements.... Then why no impact on X230?!?)

Would I just checkout this pr and build it? Or is there anything else I need to do before flashing?

Check instructions in the blobs directory. Get backups of your rom. Having an external flasher like ch431a is always a good idea to flash things back in case of a brick.

Once you have your extracted binary blobs in blobs/x220:
Build:
https://github.com/osresearch/heads-wiki/blob/master/Building.md
Basically: make BOARD=x220 CONFIG=config/x220-generic.config

Then:
https://github.com/osresearch/heads-wiki/blob/master/Installing-Heads.md
and flashing from the inside following blobs/x220/README.md or externally through flashrom.

Let us know how it goes. If things are unclear, it's a good time to do PR on the unclear stuff!
Thanks!

@tlaurion
Copy link
Collaborator

@merge

IMO this should be tested by at least one of us who didn't write the changes. Also, I think it would make sense to stash the commits together nicely and improve the commit messages.

I agree. I reviewed those changes myself and tested the outcome on a X220 and it works but does not resolve the 50 seconds delay. IMHO, it can be merged but 030 coreboot patch needs review since it only works on the x230.

Why do they mention "merging"?

The commit trace shows what was done to troubleshoot the issue.
I understand the logic with clean commits, but do not understand the strong emphasis put on it in community driven projects.

Those commit logs actually show what was done and is generally helpful for newcomers, trying to help and understand issues and troubleshooting paths. Squashing them makes it look like if the bugs were killed and make them question in issues and even reinvent the wheel, loosing precious time in the process.

As a tester, I don't look at the commit logs, just the outcome diff from master.
As a developer, I look at the commit logs to help me understand the process involved in the development of a feature or a fix.
I'm personally against squashing. But i'm open to produce cleaner commit logs myself.

In the present case, those logs show how to activate debugging features (TPM, CBMEM_TIMESTAMPS, augment cbmem log size...) which will even be helpful for new board ports.

Copy link
Collaborator

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo making gpg-gui fail

FLASHROM_OPTIONS='--force --noverify-all -p internal:laptop=force_I_want_a_brick --ifd --image bios'
;;
x220* )
FLASHROM_OPTIONS='--force --noverify-all -p internal --ifd -image bios -c MX25L6405D'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-image -> --image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credits goes to @BlackMaria :)

Copy link
Contributor Author

@snmcmillan snmcmillan May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh lord, thanks for the heads up! Fixed.

@techge
Copy link
Contributor

techge commented May 27, 2019

I am not able to build this PR. Using make board=X220 it fails reproducably while making libgcrypt for the toolchain.

2019-05-27 16:42:31+02:00 MAKE libgcrypt
tail /home/user/heads/build/log/libgcrypt.log
-----
make[3]: Leaving directory '/home/user/heads/build/libgcrypt-1.8.3/cipher'
make[2]: Leaving directory '/home/user/heads/build/libgcrypt-1.8.3/cipher'
Making install in random
make[2]: Entering directory '/home/user/heads/build/libgcrypt-1.8.3/random'
make[3]: Entering directory '/home/user/heads/build/libgcrypt-1.8.3/random'
make[3]: Nothing to be done for 'install-exec-am'.
make[3]: Nothing to be done for 'install-data-am'.
make[3]: Leaving directory '/home/user/heads/build/libgcrypt-1.8.3/random'
make[2]: Leaving directory '/home/user/heads/build/libgcrypt-1.8.3/random'
Making install in src
make[2]: Entering directory '/home/user/heads/build/libgcrypt-1.8.3/src'
/bin/sh ../libtool  --tag=CC   --mode=link /home/user/heads/install/bin/musl-gcc -fdebug-prefix-map=/home/user/heads=heads -gno-record-gcc-switches -D__MUSL__  -I//include -g -O2 -fvisibility=hidden -fno-delete-null-pointer-checks -Wall    -Wl,--version-script=./libgcrypt.vers -version-info 22:3:2  -o libgcrypt.la -rpath //lib libgcrypt_la-visibility.lo libgcrypt_la-misc.lo libgcrypt_la-global.lo libgcrypt_la-sexp.lo libgcrypt_la-hwfeatures.lo libgcrypt_la-stdmem.lo libgcrypt_la-secmem.lo libgcrypt_la-missing-string.lo libgcrypt_la-fips.lo libgcrypt_la-hmac256.lo libgcrypt_la-context.lo  hwf-x86.lo ../cipher/libcipher.la ../random/librandom.la ../mpi/libmpi.la ../compat/libcompat.la  -L//lib -lgpg-error 
libtool: link: /home/user/heads/install/bin/musl-gcc -fdebug-prefix-map=/home/user/heads=heads -gno-record-gcc-switches -D__MUSL__  -shared  -fPIC -DPIC  .libs/libgcrypt_la-visibility.o .libs/libgcrypt_la-misc.o .libs/libgcrypt_la-global.o .libs/libgcrypt_la-sexp.o .libs/libgcrypt_la-hwfeatures.o .libs/libgcrypt_la-stdmem.o .libs/libgcrypt_la-secmem.o .libs/libgcrypt_la-missing-string.o .libs/libgcrypt_la-fips.o .libs/libgcrypt_la-hmac256.o .libs/libgcrypt_la-context.o .libs/hwf-x86.o  -Wl,--whole-archive ../cipher/.libs/libcipher.a ../random/.libs/librandom.a ../mpi/.libs/libmpi.a ../compat/.libs/libcompat.a -Wl,--no-whole-archive  -L//lib -lgpg-error  -O2 -Wl,--version-script=./libgcrypt.vers   -Wl,-soname -Wl,libgcrypt.so.20 -o .libs/libgcrypt.so.20.2.3
/home/user/heads/crossgcc/x86_64-linux-musl/lib/gcc/x86_64-linux-musl/5.3.0/../../../../x86_64-linux-musl/bin/ld: /usr/lib/libc_nonshared.a(fstat.oS): unrecognized relocation (0x29) in section `.text'
/home/user/heads/crossgcc/x86_64-linux-musl/lib/gcc/x86_64-linux-musl/5.3.0/../../../../x86_64-linux-musl/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:594: libgcrypt.la] Error 1
make[2]: Leaving directory '/home/user/heads/build/libgcrypt-1.8.3/src'
make[1]: *** [Makefile:477: install-recursive] Error 1
make[1]: Leaving directory '/home/user/heads/build/libgcrypt-1.8.3'
make: *** [Makefile:377: /home/user/heads/build/libgcrypt-1.8.3/.build] Error 1

I am on Arch Linux, 64bit.

@BlackMaria
Copy link

@techge if its not a typo, board should be all caps BOARD.
This builds fine with both the debian and fedora docker images. What is your gcc/ld version?
I assume you did this but if not, can you use "make BOARD=x220 V=1" to get more verbose?

@snmcmillan
Copy link
Contributor Author

snmcmillan commented May 28, 2019 via email

@snmcmillan
Copy link
Contributor Author

snmcmillan commented May 28, 2019

@techge board needs to be capitalized. The correct command for you is make BOARD=x220

Edit: It seems that that's an actual build issue. It builds fine on a clean copy of the repo on my end.

I've tested it on Ubuntu 19.04, Debian Testing (Buster), Fedora 29, Fedora 30 and Gentoo.

@snmcmillan
Copy link
Contributor Author

snmcmillan commented May 28, 2019

I also need an additional X230 tester that can test the x230-flash build to cross-verify, and I need a T430 tester that can test t430-flash and t430 board builds, as I've converted the x230-flash.init file into a universal install.init file for all boards that make use of a dual-chip setup.

I do not have a T430 to test with, but I want someone else that has a T430 that they can put coreboot on to try it out. It will likely help on identifying the issue with the boot-time delay as well.

Currently getting x230-flash built to test it on my end.

@merge
Copy link
Contributor

merge commented May 29, 2019

I also need an additional X230 tester that can test the x230-flash build to cross-verify, and I need a T430 tester that can test t430-flash and t430 board builds, as I've converted the x230-flash.init file into a universal install.init file for all boards that make use of a dual-chip setup.

I do not have a T430 to test with, but I want someone else that has a T430 that they can put coreboot on to try it out. It will likely help on identifying the issue with the boot-time delay as well.

Currently getting x230-flash built to test it on my end.

I'd very much prefer to have one change only in a pull request. Don't change existing boards and add new ones on one PR.

@snmcmillan
Copy link
Contributor Author

@merge I can attempt to separate out the changes, but the T430 addition relies on the changes I made to X230.

@merge
Copy link
Contributor

merge commented May 30, 2019

@merge I can attempt to separate out the changes, but the T430 addition relies on the changes I made to X230.

If that is the case then X230 changes are bugfixes anyways and will be merged quickly. why not do them seperately in a PR before adding a new board? Try to keep a PR simple if possible.

@snmcmillan
Copy link
Contributor Author

The PR is now split. I will close this one and the others can be looked at.

@snmcmillan snmcmillan closed this Jun 7, 2019
@techge
Copy link
Contributor

techge commented Jun 7, 2019

#578 #579 and #580

@flawedworld
Copy link
Contributor

@techge Did you find out a solution to the build failing? I also am on Arch 64 bit and get the same error on the X230, T430, assuming it's a build thing for sure.

@techge
Copy link
Contributor

techge commented Jun 16, 2019

@flawedworld Unfortunately, I didn't. I planned to just grab a VM and build it there, but actually I got distracted by other stuff to do and didn't even did that. :(

@flawedworld
Copy link
Contributor

@techge nah its fine, funnily enough I was doing a T430 port last year and well, life happened, saw this yesterday and I reminded myself about it. I'll try a Debian VM.

@flawedworld
Copy link
Contributor

@SebastianMcMillan I am happy to test out any T430 builds, I may as well help you with what I never could be bothered to finish.

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Jun 17, 2019

@flawedworld current PR #580 is where you should start. Build that branch.

Also, find me on the u-root slack, where we can do a private conversation.

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 18, 2019

@SebastianMcMillan : Just a quick insight about ME delaying boot.

Could you try to extract the ifd, me as per this guide, and reinject the binaries directly from coreboot config and see if it resolves the 50 seconds delay?

That is what i'm planning to do to resolve the 12MB internal flashing requirement from x230 board config and it works for it. Non-obstant the current musl blocker, if you don't checkout clean and have musl already compiled, you should be able to build and report. I have intuition that it could be the difference between x230 working and others not working... The ifd in x230 being faked, and x230 only reflashing the bios region of the ROM, not ever touching ME which sits on its own SPI, containing the original IFD.

Let me know!

@snmcmillan
Copy link
Contributor Author

snmcmillan commented Nov 19, 2019 via email

@techge
Copy link
Contributor

techge commented Jan 3, 2020

Could you try to extract the ifd, me as per this guide, and reinject the binaries directly from coreboot config and see if it resolves the 50 seconds delay?

I followed the guide and truncated the ME, still the boot delay is valid.

@tlaurion
Copy link
Collaborator

When inserting expended IFD and neutered+deactivated ME inside of the x230 and changing CBFS region to fit expended size, I get the 50 seconds delay on x230 also. When putting back
CONFIG_CBFS_SIZE=0x700000
inside of coreboot config, the 50 seconds delay disappears. Just a hint.

@techge
Copy link
Contributor

techge commented Jan 20, 2020

Very interesting! I try to test it as soon as possible 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants